Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IBX-7603: Added missing subfield for ezurl #137

Open
wants to merge 6 commits into
base: 2.3
Choose a base branch
from
Open

IBX-7603: Added missing subfield for ezurl #137

wants to merge 6 commits into from

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented Feb 5, 2024 •
edited
Loading

Jira: https://issues.ibexa.co/browse/IBX-7603

If the ibexa.graphql.schema.should.extend.ezurl parameter is false (default), the graphQL query will look like before, i.e.:

{
  content {
    urlCT(contentId:68) {
      urlField
    }
  }
}

but if we set this parameter to true, the graphQL query will look like this:

{
  content {
    urlCT(contentId:68) {
      urlField {
        link,
        text
      }
    }
  }
}

@mateuszdebinski mateuszdebinski added Bug Something isn't working Ready for review labels Feb 5, 2024
@mateuszdebinski mateuszdebinski requested review from bdunogier and a team February 5, 2024 15:13
@mateuszdebinski mateuszdebinski self-assigned this Feb 5, 2024
@konradoboza
Copy link
Member

@mateuszdebinski could you please provide example query that will no longer work? The description is close to non-existent, it's hard to tackle the exact problem judging only by the schema adjustment.

@mateuszdebinski
Copy link
Contributor Author

@mateuszdebinski could you please provide example query that will no longer work? The description is close to non-existent, it's hard to tackle the exact problem judging only by the schema adjustment.

@konradoboza I added what it looks like now and what it will look like after the change, The current query will not work without specifying what fields you want to extract from the query

konradoboza
konradoboza approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for providing examples @mateuszdebinski, now the topic is clear. While it's technically a BC break, I see that we are lacking UrlFieldValue anyway since that's how we handle fieldtypes in general when it comes to GraphQL schema. From my perspective it's an acceptable solution, but want to poke @bdunogier and maybe @alongosz for their opinions. An alternative that would spare us BC concerns would be creating another dedicated GraphQL endpoint though.

@konradoboza konradoboza requested review from alongosz and a team March 7, 2024 09:56
@bdunogier
Copy link
Member

creating another dedicated GraphQL endpoint though.
We can't really do that, the graphql endpoint is kind of "unique".

But BC is a problem here. If a project uses an ezurl field over GraphQL, their existing query will break, period. I don't think it's acceptable.

GraphQL doesn't have anything for changing a primitive type to a custom one, afaik. The only way I can think of is some kind of feature flag. We add a custom mapper for that field type, and in the mapper, we either String or UrlFieldValue depending on the flag.

As it doesn't seem very likely that this is widely used, and in order to maximize the adoption of the new schema, we could disable the BC flag by default, and document that it must be enabled if such a field type is used. Of course, updating the client code is a better option.

bdunogier
bdunogier requested changes Mar 7, 2024
View reviewed changes
Copy link
Member

@bdunogier bdunogier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BC concern, see #137 (comment).

@alongosz
Copy link
Member

alongosz commented Mar 7, 2024

@bdunogier I don't know much about GraphQL. Is it possible to keep here BC? Meaning when old query is passed, returning old result? We need to have a solution here.

@bdunogier
Copy link
Member

Unfortunately, no, @alongosz, no BC features for changing a primitive type to an object. Hence my suggestion above for a feature flag.

@bdunogier
Copy link
Member

Let's complete this one, it's not gonna fix itself :)

The way to fix it is correct, but the BC break isn't. We can change the schema that way in v5.0, but not in 4.6. It is likely that this ezurl is used with GraphQL by existing apps.

We must add a feature flag / configuration directive that enables the new format, and ship it as disabled on 4.x. On 5.0, we can either ship the feature flag and enable it by default, or just change the schema. In any case, we should add a @deprecated flag to the current version of the field.

Is that okay for everybody ?

@mateuszdebinski mateuszdebinski force-pushed the IBX-7603_missing_text_field_from_url branch from 54e7437 to f0db3d6 Compare July 15, 2024 13:24
@mateuszdebinski
Copy link
Contributor Author

As we do not recommend using a lower PHP version than 8.1 and ultimately 7.4, I changed the dependency in composer for PHP to at least 7.4, I don't know if I can do that, so please let me know whether to restore support for 7.3

@adamwojs
Copy link
Member

Sorry, but we need to keep compatibility with PHP 7.3. Bumping minimum required version of package is a BC break.

@mateuszdebinski mateuszdebinski force-pushed the IBX-7603_missing_text_field_from_url branch from f4f21cd to d41943d Compare July 16, 2024 10:14
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alongosz alongosz Awaiting requested review from alongosz

@bdunogier bdunogier Awaiting requested review from bdunogier

@konradoboza konradoboza Awaiting requested review from konradoboza

Assignees

@webhdx webhdx

@tischsoic tischsoic

@konradoboza konradoboza

@mateuszdebinski mateuszdebinski

Labels
Bug Something isn't working Ready for review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mateuszdebinski @konradoboza @bdunogier @alongosz @adamwojs @webhdx @tischsoic